fix(install): unwrap lspServers envelope in plugin .lsp.json (closes #1683)#1686
Conversation
When a plugin ships a .lsp.json using the { "lspServers": { ... } }
wrapper format, _read_lsp_json() now detects and unwraps the envelope
instead of treating "lspServers" as a server name.
This fixes the silent skip with:
Skipping invalid LSP server 'lspServers': requires 'command'
The flat format (server names as top-level keys) continues to work
unchanged.
Fixes #1683
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
This PR fixes apm install handling of plugin-provided .lsp.json files by supporting the common { "lspServers": { ... } } wrapper format, so wrapped LSP server definitions are correctly discovered and converted into dependencies instead of being misinterpreted and skipped.
Changes:
- Update
_read_lsp_json()to unwrap the"lspServers"envelope when present. - Expand unit tests to cover wrapped
.lsp.jsonparsing, auto-discovery behavior, and end-to-end conversion into valid LSP dependencies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/apm_cli/deps/plugin_parser.py |
Adds unwrap logic for the "lspServers" envelope and updates the docstring to document both supported formats. |
tests/unit/deps/test_plugin_parser_lsp.py |
Adds new tests covering wrapped parsing, auto-discovery with wrapper, and end-to-end dependency generation. |
Only unwrap the envelope when all values in the inner dict are dicts (i.e. it looks like a server map). A flat-format server literally named 'lspServers' would have scalar values like 'command', so the all-dicts check avoids mis-detecting it as an envelope. Adds a regression test for this ambiguous edge case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses panel follow-ups for PR #1686 by proving wrapped plugin .lsp.json files produce dependencies.lsp entries without warning and documenting the accepted plugin file shapes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
removed
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean, minimal change; unwrap heuristic distinguishes envelope from flat server named lspServers and tests cover core paths. |
| CLI Logging Expert | 0 | 0 | 1 | Clean CLI-output change; valid wrapper is silent by default and debug trace is appropriate for verbose troubleshooting. |
| DevX UX Expert | 0 | 0 | 0 | User-transparent compatibility fix; plugin authors can keep native Copilot/Claude LSP shape without warnings. |
| Supply Chain Security Expert | 0 | 0 | 1 | No supply-chain concerns; no new file I/O, path joins, credential access, or execution surface. |
| OSS Growth Hacker | 0 | 0 | 1 | Community-friendly compatibility fix; docs now mention both plugin .lsp.json shapes. |
| Doc Writer | 0 | 0 | 2 | Docs are accurate, concise, and consistent with the parser. |
| Test Coverage Expert | 0 | 0 | 0 | Regression-trap coverage is strong: 22/22 parser tests pass and new tests cover warning absence plus apm.yml deps synthesis. |
B/R/N are signal counts, not gates. The maintainer ships.
Recommendation
Merge when ready. The follow-ups surfaced in the first panel pass were folded: warning-absence coverage, synthesized dependencies.lsp coverage, debug traceability, and concise docs/apm-guide notes. Remaining panel observations are optional nits and do not need tracking.
Folded in this run
- (panel) Assert valid wrapped
.lsp.jsonemits no WARNING-level records -- resolved in 096baa6. - (panel) Prove wrapped plugin
.lsp.jsonis synthesized intodependencies.lsp-- resolved in 096baa6. - (panel) Add debug trace when the envelope is unwrapped -- resolved in 096baa6.
- (panel) Document plugin
.lsp.jsonaccepts flat map or{ "lspServers": { ... } }envelope in docs and apm-guide -- resolved in 096baa6.
Copilot signals reviewed
No Copilot inline comments were present after two fetch rounds. The bot's overview comment was reviewed; it raised no actionable inline findings.
Regression-trap evidence (mutation-break gate)
tests/unit/deps/test_plugin_parser_lsp.py::TestReadLspJson::test_unwraps_lsp_servers_envelope_without_warning-- deleted_read_lsp_jsonunwrap guard by forcing the condition false; test FAILED as expected; guard restored.tests/unit/deps/test_plugin_parser_lsp.py::TestLspServersToApmDeps::test_wrapped_lsp_json_is_written_to_apm_yml_deps-- deleted_read_lsp_jsonunwrap guard by forcing the condition false; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both passed before push. Additional local guards also passed: pylint R0801 and scripts/lint-auth-signals.sh.
CI
gh pr checks 1686 --repo microsoft/apm reports all checks successful: 14 successful, 1 skipped, 0 failing, 0 pending (after 0 CI fix iteration(s)).
Mergeability status
Captured from gh pr view 1686 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1686 | 096baa6 |
ship_now | 1 | 4 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
1 outer iteration; 2 Copilot round(s). Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] Sibling metadata keys beside
lspServersare ignored when the envelope is unwrapped atsrc/apm_cli/deps/plugin_parser.py:503.
Not a correctness issue because common siblings such as$schemaare metadata; parser returns the server map by design.
CLI Logging Expert
- [nit] Debug message logs the full path at
src/apm_cli/deps/plugin_parser.py:511.
Verbose output may be noisier with absolute paths, but this is consistent with existing parser diagnostics.
DevX UX Expert
No findings.
Supply Chain Security Expert
- [nit] Top-level shallow copy leaves inner config dicts shared at
src/apm_cli/deps/plugin_parser.py:511.
Not a security issue because callers do not mutate the parsed config; defensive deep copy would be optional future hardening.
OSS Growth Hacker
- [nit] Docs could include a tiny copy-paste wrapped
.lsp.jsonexample atdocs/src/content/docs/consumer/install-lsp-servers.md:168.
The current concise sentence is enough for this bug-fix scope; avoid bloat unless a future docs pass expands examples.
Auth Expert -- inactive
PR touches plugin parser, LSP parser tests, and LSP docs only; no auth, token, credential, host-classification, or git/HTTP auth surface is affected.
Doc Writer
- [nit] apm-guide sentence shifts from target-write behavior to plugin-ingestion behavior at
packages/apm-guide/.apm/skills/apm-usage/dependencies.md:382.
Wording is accurate and brief; optional phrasing can wait for a docs sweep. - [nit] Docs could say which runtime shape APM emits at
docs/src/content/docs/consumer/install-lsp-servers.md:168.
Non-essential here because output shapes are already covered by the runtime table.
Test Coverage Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
danielmeppiel
deleted the
sergio-sisternes-epam/fix-lsp-json-unwrap-lspservers-envelope
branch
TL;DR
Plugin
.lsp.jsonfiles using the{ "lspServers": { ... } }wrapper format are now correctly unwrapped during install, instead of being silently skipped with a misleading validation error.Problem
When a plugin ships a
.lsp.jsonusing the standard wrapper format:{ "lspServers": { "my-lsp-server": { "command": "my-lsp", "args": ["--stdio"], "extensionToLanguage": { ".ext": "mylang" } } } }_read_lsp_json()returned the parsed JSON verbatim, causing"lspServers"to be treated as a server name. Its nested dict value (containing the actual servers) was then treated as that server's config -- which lacks acommandfield, producing:The real LSP servers were never written to
.github/lsp.json.Fix
In
_read_lsp_json()(src/apm_cli/deps/plugin_parser.py), after parsing the JSON dict, detect whether it contains a"lspServers"key with a dict value. If so, return the inner dict (the actual server map). The flat format (server names as top-level keys) continues to work unchanged.This mirrors the envelope handling already used by
LSPIntegrator._write_target_config()when writing to Copilot and Claude config files.Changes
src/apm_cli/deps/plugin_parser.pytests/unit/deps/test_plugin_parser_lsp.pyTests added
test_unwraps_lsp_servers_envelope-- wrapped format returns inner serverstest_flat_format_still_works-- flat format regression guardtest_auto_discovery_with_lsp_servers_wrapper-- auto-discovered.lsp.jsonwith wrappertest_wrapped_lsp_json_produces_valid_deps-- end-to-end: wrapped file -> valid depsValidation
tests/unit/deps/test_plugin_parser_lsp.pyruff check+ruff format --checkcleanpylint R0801(duplication) cleanFixes #1683